Repository navigation
policy: chassis core — omnigraph-policy crate + Omnigraph::enforce() (MR-722) - #102
Merged
Merged
Conversation
…(MR-722) PR #2 of the policy chassis series (PR #1 = MR-731, merged in #101). The structural fix that moves Cedar enforcement from HTTP-only to engine-wide. apply_schema is the proof-of-concept writer; PR #3 fans the enforce() call out to the remaining six (mutate_as, load, ingest_as, branch_create_from, branch_delete, branch_merge). ## What lands ### New crate: omnigraph-policy The 844-line policy.rs moves from `omnigraph-server` into a new `omnigraph-policy` workspace crate so both engine and server can depend on it. Cedar dependency moves with it. The server's policy.rs becomes a re-export shim (`pub use omnigraph_policy::*`) so existing `omnigraph_server::PolicyAction` etc. paths keep working — CLI and test consumers don't have to migrate in one go. ### New trait: PolicyChecker ```rust pub trait PolicyChecker: Send + Sync { fn check(&self, action: PolicyAction, scope: &ResourceScope, actor: &str) -> Result<(), PolicyError>; } ``` `PolicyEngine` (Cedar-backed) implements it. `Omnigraph::with_policy()` takes `Arc<dyn PolicyChecker>`. Engine tests mock the trait without spinning up Cedar. MR-725 will extend the trait with `predicate_for()` for query-layer pushdown — additive, no call-site changes. ### New enum: ResourceScope Four variants — Graph, Branch, TargetBranch, BranchTransition — mapping cleanly to today's `(branch, target_branch)` shape on PolicyRequest via `to_branch_pair()`. Each engine writer picks the variant that matches the existing HTTP-layer convention so engine and HTTP evaluate the same Cedar decision. **Invariant**: ResourceScope stays at branch granularity. Per-type and per-row scope are MR-725's territory, not engine-layer's. Adding Type/Row variants here creates two places per-type policy can be evaluated, which can drift. See chassis design refinements comment on MR-722 (2026-05-17). ### Omnigraph::with_policy() + enforce() * New `policy: Option<Arc<dyn PolicyChecker>>` field on Omnigraph, None by default (preserves embedded/dev no-enforcement mode). * `with_policy(self, checker)` setter — builder-style, consumes self. * `enforce(action, scope, actor)` — the gate. When policy is None, no-op. When policy is Some AND actor is None, hard error — silent bypass via "I forgot the actor" is exactly the footgun this gate is here to prevent. ### apply_schema_as: first writer wired * New public method `apply_schema_as(source, options, actor)` that calls `enforce(SchemaApply, TargetBranch("main"), actor)` before acquiring the schema-apply lock or doing any other work. * Existing `apply_schema(source)` and `apply_schema_with_options(...)` delegate to it with actor=None (no-actor variants). * HTTP handler `server_schema_apply` updated to call apply_schema_as with the resolved actor. AppState construction injects the PolicyEngine into Omnigraph via `with_policy`. HTTP-layer authorize_request still fires first; the engine gate is the redundant-but-correct backstop and the only path that protects SDK / embedded callers. PR #3 removes the HTTP redundancy. ### OmniError::Policy New error variant for engine-layer policy denial / evaluation failure. ApiError::from_omni maps it to 403. ### MR-724 Admin action — Option A reservation PolicyAction::Admin kept in the enum with a load-bearing doc comment naming its future consumers (hot reload, audit log query, approvals list per MR-726 / MR-732 / MR-734). No enforce(Admin, ...) call site exists yet — the variant is reserved so the action vocabulary is complete from chassis day one. MR-724 closes when the first consumer surface ships. ### New SDK-side integration test `crates/omnigraph/tests/policy_engine_chassis.rs` — four tests covering: * Policy denies for unauthorized actor → OmniError::Policy * Policy permits for authorized actor → apply succeeds * Policy installed + no actor → hard error (forget-the-actor footgun) * No policy → no-op (embedded/dev default still works) These exercise the engine path directly — no HTTP layer involved. ## Test results - cargo test --workspace --locked --no-fail-fast: 851 passed, 0 failed * 45 server tests (existing) pass * 14 schema_apply tests (existing) pass * 4 new chassis tests pass * 60 OpenAPI tests pass (no HTTP API surface changes) * No regressions across the workspace ## Architectural decisions baked in Per MR-722 chassis design refinements comment (2026-05-17): 1. PolicyChecker is a trait, not just a concrete. Engine and server consume the trait. MR-725 adds predicate_for() additively. 2. ResourceScope stays at branch granularity. No Type/Row variants. 3. Coarse-vs-fine framing pinned: engine-layer is action gate; query-layer (MR-725) is predicate gate. Both backed by same Cedar engine; non-overlapping responsibilities. 4. Admin action reserved for policy-management surfaces (MR-724 Option A). ## Pending follow-ups (PR #3+) - Fan-out enforce() to mutate_as, load, ingest_as, branch_create_from, branch_delete, branch_merge (PR #3). - Remove HTTP-layer authorize_request redundancy once engine gate covers all writers (PR #3). - CLI policy injection into Omnigraph for non-`policy validate|test|explain` subcommands (PR #3 or follow-up). - MR-723 default-deny 3-state matrix (PR #4). - MR-736 severity warn/deny (PR #5). - AGENTS.md scope-of-enforcement rewrite once chassis fully lands. - Coarse-vs-fine framing in docs/user/policy.md. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
2 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/omnigraph/src/db/omnigraph.rs">
<violation number="1" location="crates/omnigraph/src/db/omnigraph.rs:406">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Doc comment on `apply_schema_as` incorrectly claims `enforce(SchemaApply, Branch("main"), actor)` is used; the actual callee uses `TargetBranch("main")`, creating a misleading claim/implementation mismatch.</violation>
</file>
<file name="crates/omnigraph/src/error.rs">
<violation number="1" location="crates/omnigraph/src/error.rs:94">
P2: `OmniError::Policy(String)` loses policy error type information, so denial and policy-engine internal failures cannot be handled differently.</violation>
</file>
Linked issue analysis
Linked issue: MR-722: Policy chassis: engine-layer enforcement + identity invariant + default-deny + action vocabulary
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Add omnigraph-policy workspace crate (move policy code out of server) and re-export shim from omnigraph-server | crates/omnigraph-policy added with Cargo.toml and src/lib.rs; omnigraph-server policy.rs replaced by a pub use shim; workspace Cargo.toml and Cargo.lock updated. |
| ✅ | Introduce PolicyChecker trait and ResourceScope enum as the engine-layer enforcement API | PolicyChecker trait and ResourceScope are defined in the new omnigraph-policy crate and implemented by PolicyEngine; trait is used by engine-side code. |
| ✅ | Add Omnigraph::with_policy(...) setter and a policy field on Omnigraph | Omnigraph gained a policy: Option> field and a with_policy(...) builder-style setter that stores the checker. |
| ✅ | Add Omnigraph::enforce(...) gate that calls the PolicyChecker and errors when a policy is configured but no actor is provided | enforce() implemented to no-op when none, to return a clear OmniError::Policy when actor is None, and to map PolicyChecker results to OmniError::Policy. |
| ✅ | Wire a proof-of-concept writer: add apply_schema_as(...) and call enforce() before schema apply work | apply_schema and apply_schema_with_options delegate to apply_schema_as; schema_apply path calls db.enforce(...) with ResourceScope::TargetBranch("main") before taking locks and applying the schema. |
| ✅ | Server: inject PolicyEngine into AppState and use the engine-side apply_schema_as path (HTTP handler passes actor through) | AppState construction clones policy engine into an Arc and calls db.with_policy; server_schema_apply now calls apply_schema_as with the resolved actor; PolicyEngine re-export and dependency changes present. |
| ✅ | Add OmniError::Policy and map engine policy denials to 403 in API layer | OmniError includes a Policy(String) variant; ApiError::from_omni maps OmniError::Policy to forbidden; server paths use ApiError::from_omni for engine errors. |
| ✅ | Add SDK-side integration tests that exercise engine-layer enforcement (no HTTP) | New tests exercise with_policy + apply_schema_as: allow/deny cases, no-actor denial when policy installed, and no-policy success path; they run against the engine directly. |
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
| /// Apply a schema migration with an explicit actor for engine-layer | ||
| /// policy enforcement (MR-722). When a `PolicyChecker` is installed | ||
| /// via [`Self::with_policy`], this method calls `enforce(SchemaApply, | ||
| /// Branch("main"), actor)` before any apply work happens. Denial |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
Doc comment on apply_schema_as incorrectly claims enforce(SchemaApply, Branch("main"), actor) is used; the actual callee uses TargetBranch("main"), creating a misleading claim/implementation mismatch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph/src/db/omnigraph.rs, line 406:
<comment>Doc comment on `apply_schema_as` incorrectly claims `enforce(SchemaApply, Branch("main"), actor)` is used; the actual callee uses `TargetBranch("main")`, creating a misleading claim/implementation mismatch.</comment>
<file context>
@@ -331,7 +397,26 @@ impl Omnigraph {
+ /// Apply a schema migration with an explicit actor for engine-layer
+ /// policy enforcement (MR-722). When a `PolicyChecker` is installed
+ /// via [`Self::with_policy`], this method calls `enforce(SchemaApply,
+ /// Branch("main"), actor)` before any apply work happens. Denial
+ /// returns `OmniError::Policy` and leaves the manifest untouched.
+ ///
</file context>
Suggested change
| /// Branch("main"), actor)` before any apply work happens. Denial | |
| + /// TargetBranch("main"), actor)` before any apply work happens. Denial |
| /// denials to 403 and evaluation failures to 500; CLI and embedded | ||
| /// callers can match on this variant directly. | ||
| #[error("policy: {0}")] | ||
| Policy(String), |
There was a problem hiding this comment.
P2: OmniError::Policy(String) loses policy error type information, so denial and policy-engine internal failures cannot be handled differently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph/src/error.rs, line 94:
<comment>`OmniError::Policy(String)` loses policy error type information, so denial and policy-engine internal failures cannot be handled differently.</comment>
<file context>
@@ -85,6 +85,13 @@ pub enum OmniError {
+ /// denials to 403 and evaluation failures to 500; CLI and embedded
+ /// callers can match on this variant directly.
+ #[error("policy: {0}")]
+ Policy(String),
}
</file context>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // bearer) from 403 (policy denial), so by the time the | ||
| // engine gate fires, the bearer is valid — any failure from | ||
| // the engine is a policy outcome, not an auth one. | ||
| OmniError::Policy(message) => Self::forbidden(message), |
Contributor
There was a problem hiding this comment.
🟡 OmniError::Policy maps evaluation failures to 403 instead of 500
The OmniError::Policy variant wraps both policy denials (PolicyError::Denied) and evaluation failures (PolicyError::Internal), but from_omni at crates/omnigraph-server/src/lib.rs:469 maps all OmniError::Policy to Self::forbidden() (HTTP 403). This means a Cedar evaluation failure (malformed rule, internal error) — which is a server-side bug — is surfaced to the HTTP client as 403 Forbidden instead of 500 Internal Server Error. The variant's own docstring at crates/omnigraph/src/error.rs:90-91 claims "The HTTP layer maps denials to 403 and evaluation failures to 500", contradicting the actual from_omni behavior. For comparison, the HTTP-layer authorize_request path at crates/omnigraph-server/src/lib.rs:734 correctly maps evaluation failures to ApiError::internal (500). The discrepancy means the engine-layer enforcement path returns a misleading status code for configuration errors.
Prompt for agents
The OmniError::Policy variant wraps both policy denials and evaluation failures as a flat String. The from_omni mapping at crates/omnigraph-server/src/lib.rs:469 maps all Policy errors to forbidden (403). To fix this properly:
1. Option A (minimal): Since the Display impl of PolicyError prefixes denial messages with "policy denied:" and evaluation failures with "policy evaluation failed:", from_omni could match on the message prefix to dispatch to forbidden vs internal. This is fragile string matching though.
2. Option B (structural): Change OmniError::Policy to carry a boolean or enum distinguishing denial from internal failure, e.g. Policy { denied: bool, message: String } or split into PolicyDenied(String) and PolicyInternal(String). Then from_omni maps PolicyDenied to forbidden and PolicyInternal to internal.
3. Also update the docstring on OmniError::Policy in crates/omnigraph/src/error.rs:88-92 to accurately describe what from_omni does with this variant.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+88
to
+94
| /// Engine-layer policy enforcement (MR-722). Wraps either a policy | ||
| /// denial ("you can't do that") or a policy-evaluation failure | ||
| /// ("the policy engine itself blew up"). The HTTP layer maps | ||
| /// denials to 403 and evaluation failures to 500; CLI and embedded | ||
| /// callers can match on this variant directly. | ||
| #[error("policy: {0}")] | ||
| Policy(String), |
Contributor
There was a problem hiding this comment.
🟡 OmniError::Policy docstring falsely claims evaluation failures map to 500
The docstring on the Policy variant says "The HTTP layer maps denials to 403 and evaluation failures to 500" but the actual from_omni implementation at crates/omnigraph-server/src/lib.rs:469 maps all OmniError::Policy to Self::forbidden() (403). This violates AGENTS.md rule 6 ("Don't lie") — the doc describes behavior the code does not implement. The comment at crates/omnigraph-server/src/lib.rs:463-468 correctly says "All denials and evaluation failures surface here as 403", contradicting the error variant's own doc.
Suggested change
| /// Engine-layer policy enforcement (MR-722). Wraps either a policy | |
| /// denial ("you can't do that") or a policy-evaluation failure | |
| /// ("the policy engine itself blew up"). The HTTP layer maps | |
| /// denials to 403 and evaluation failures to 500; CLI and embedded | |
| /// callers can match on this variant directly. | |
| #[error("policy: {0}")] | |
| Policy(String), | |
| /// Engine-layer policy enforcement (MR-722). Wraps either a policy | |
| /// denial ("you can't do that") or a policy-evaluation failure | |
| /// ("the policy engine itself blew up"). The HTTP `from_omni` path | |
| /// maps both to 403; CLI and embedded callers can match on this | |
| /// variant directly. | |
| #[error("policy: {0}")] | |
| Policy(String), |
Was this helpful? React with 👍 or 👎 to provide feedback.
| "crates/omnigraph-compiler", | ||
| "crates/omnigraph", | ||
| "crates/omnigraph-cli", | ||
| "crates/omnigraph-policy", |
Contributor
There was a problem hiding this comment.
🟡 AGENTS.md workspace crates list not updated for new omnigraph-policy crate
The PR adds omnigraph-policy to the workspace in Cargo.toml (line 7) but does not update AGENTS.md line 20 which lists "Workspace crates: omnigraph-compiler, omnigraph (engine), omnigraph-cli, omnigraph-server" — missing the new crate. This violates AGENTS.md rule 1: "Update in the same PR. New endpoint, query function, CLI flag, env var, constant, schema construct, or invariant: update both the source code and the doc in the same change."
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+335
to
+338
| pub fn with_policy(mut self, checker: Arc<dyn omnigraph_policy::PolicyChecker>) -> Self { | ||
| self.policy = Some(checker); | ||
| self | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 docs/dev/invariants.md Known Gap about policy enforcement at HTTP boundary not updated
The PR adds engine-layer policy enforcement (Omnigraph::enforce(), PolicyChecker trait, with_policy()) but does not update docs/dev/invariants.md:133 which says "Cedar enforcement is at the HTTP boundary today." This known gap is now partially addressed — engine-layer enforcement exists for apply_schema_as, with remaining writers planned for PR #3. Per AGENTS.md rule 1, docs must be updated in the same PR when an invariant or known gap changes.
Was this helpful? React with 👍 or 👎 to provide feedback.
4 tasks done
aaltshuler
added a commit
that referenced
this pull request
May 18, 2026
…-722) (#103) PR #102 wired apply_schema_as. This PR completes the chassis-side coverage so every public mutating engine entry point hits the same Omnigraph::enforce(action, scope, actor) gate regardless of transport: - mutate_as → enforce(Change, Branch(branch), actor) - load_as → enforce(Change, Branch(branch), actor) - ingest_as → enforce(Change, Branch(branch), actor); also threads actor through the implicit branch_create_from_as so fresh-branch ingest correctly hits BranchCreate too - branch_create_as → enforce(BranchCreate, TargetBranch(name), actor) - branch_create_from_as → enforce(BranchCreate, BranchTransition { source, target }, actor) - branch_delete_as → enforce(BranchDelete, TargetBranch(name), actor) - branch_merge_as → enforce(BranchMerge, BranchTransition { source, target }, actor) Three new _as variants for branch ops (create, create_from, delete) that had no actor surface before; existing actor-less variants delegate with actor=None so the no-policy path is a strict no-op. HTTP handlers updated to thread the resolved actor into the new _as variants for branch_create and branch_delete (was previously dropped). 14 new SDK chassis tests (one allow + one deny pair per wired writer); the existing 4 apply_schema_as tests stay. All 18 pass. docs/user/policy.md updated to describe engine-wide enforcement and the coarse-vs-fine layer split (engine = action gate, query layer per-row = MR-725 future). AGENTS.md capability matrix updated to match. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #2 of the policy chassis series (PR #1 = MR-731, merged in #101). The structural fix that moves Cedar enforcement from HTTP-only to engine-wide.
apply_schemais the proof-of-concept writer; PR #3 fans theenforce()call out to the remaining six writers.What lands
New crate:
omnigraph-policyThe 844-line
policy.rsmoves fromomnigraph-serverinto a newomnigraph-policyworkspace crate so both engine and server can depend on it. Cedar dependency moves with it. The server'spolicy.rsbecomes a re-export shim (pub use omnigraph_policy::*) so existingomnigraph_server::PolicyActionetc. paths keep working — CLI and test consumers don't have to migrate in one go.New trait:
PolicyCheckerPolicyEngine(Cedar-backed) implements it.Omnigraph::with_policy()takesArc<dyn PolicyChecker>. Engine tests can mock the trait without spinning up Cedar. MR-725 will extend the trait withpredicate_for()for query-layer pushdown — additive, no call-site changes.New enum:
ResourceScopeFour variants —
Graph,Branch,TargetBranch,BranchTransition— mapping cleanly to today's(branch, target_branch)shape onPolicyRequestviato_branch_pair(). Each engine writer picks the variant that matches the existing HTTP-layer convention so engine and HTTP evaluate the same Cedar decision.Invariant: ResourceScope stays at branch granularity. Per-type and per-row scope are MR-725's territory, not engine-layer's. See chassis design refinements comment on MR-722 (2026-05-17).
Omnigraph::with_policy()+enforce()policy: Option<Arc<dyn PolicyChecker>>field onOmnigraph.Noneby default (preserves embedded/dev no-enforcement mode).with_policy(self, checker)setter — builder-style, consumesself.enforce(action, scope, actor)— the gate. When policy isNone, no-op. When policy isSomeAND actor isNone, hard error — silent bypass via "I forgot the actor" is exactly the footgun this gate is here to prevent.apply_schema_as: first writer wiredapply_schema_as(source, options, actor)that callsenforce(SchemaApply, TargetBranch("main"), actor)before acquiring the schema-apply lock.apply_schema(source)andapply_schema_with_options(...)delegate withactor=None.server_schema_applyupdated to callapply_schema_aswith the resolved actor.AppStateconstruction injects thePolicyEngineintoOmnigraphviawith_policy. Both the HTTP-layerauthorize_requestAND the engine gate fire for now — engine gate is the redundant-but-correct backstop and the only path that protects SDK / embedded callers. PR Add support for null literals in query parameters #3 removes the HTTP redundancy.OmniError::PolicyNew error variant for engine-layer policy denial / evaluation failure.
ApiError::from_omnimaps it to 403.MR-724
Adminaction — Option A reservationPolicyAction::Adminkept in the enum with a load-bearing doc comment naming its future consumers (hot reload, audit log query, approvals list per MR-726 / MR-732 / MR-734). Noenforce(Admin, ...)call site exists yet — the variant is reserved so the action vocabulary is complete from chassis day one. MR-724 closes when the first consumer surface ships.New SDK-side integration test
crates/omnigraph/tests/policy_engine_chassis.rs— four tests covering:OmniError::PolicyThese exercise the engine path directly — no HTTP layer involved.
Test plan
cargo test --workspace --locked --no-fail-fast— 851 passed, 0 failedscripts/check-agents-md.sh— passesArchitectural decisions baked in
Per MR-722 chassis design refinements comment (2026-05-17):
PolicyCheckeris a trait, not just a concrete. Engine and server consume the trait. MR-725 addspredicate_for()additively.ResourceScopestays at branch granularity. NoType/Rowvariants — MR-725's territory.Adminaction reserved for policy-management surfaces (MR-724 Option A).Pending follow-ups (PR #3+)
enforce()tomutate_as,load,ingest_as,branch_create_from,branch_delete,branch_merge(PR Add support for null literals in query parameters #3)authorize_requestredundancy once engine gate covers all writers (PR Add support for null literals in query parameters #3)Omnigraphfor non-policy validate|test|explainsubcommands (PR Add support for null literals in query parameters #3 or follow-up)docs/user/policy.md🤖 Generated with Claude Code